test(core): remove FsAdapter#2979
test(core): remove FsAdapter#2979iuioiua wants to merge 21 commits intodenoland:mainfrom iuioiua:remove-FsAdapter
FsAdapter#2979Conversation
| try { | ||
| for await ( | ||
| const entry of walk(islandDir, { | ||
| includeDirs: false, | ||
| exts: ["tsx", "jsx", "ts", "js"], | ||
| skip, | ||
| }) | ||
| ) { | ||
| islandPaths.push(entry.path); | ||
| } | ||
| } catch (error) { | ||
| if (!(error instanceof Deno.errors.NotFound)) { | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| for await ( | ||
| const entry of internals.walk(routesDir, { | ||
| includeDirs: false, | ||
| exts: ["tsx", "jsx", "ts", "js"], | ||
| skip, | ||
| }) | ||
| ) { | ||
| const relative = path.relative(routesDir, entry.path); | ||
|
|
||
| // A `(_islands)` path segment is a local island folder. | ||
| // Any route path segment wrapped in `(_...)` is ignored | ||
| // during route collection. | ||
| const match = relative.match(GROUP_REG); | ||
| if (match && match[2][0] === "_") { | ||
| if (match[2] === "_islands") { | ||
| islandPaths.push(entry.path); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| const url = new URL(relative, "http://localhost/"); | ||
| relRoutePaths.push(url.pathname.slice(1)); | ||
| }, | ||
| ignore, | ||
| fs, | ||
| ), | ||
| ]); | ||
| const url = new URL(relative, "http://localhost/"); | ||
| relRoutePaths.push(url.pathname.slice(1)); | ||
| } | ||
| } catch (error) { | ||
| // `islandDir` or `routesDir` does not exist, so we can skip it | ||
| if (!(error instanceof Deno.errors.NotFound)) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Here, instead of checking whether the first argument passed to walk() is a directory, we just simply try to walk it. If it's not a directory, a Deno.errors.NotFound error is thrown and we ignore it. This has the same functionality as before, but just without having to use some isDirectory() function.
There was a problem hiding this comment.
Is there any performance gain from avoiding the isDirectory function?
There was a problem hiding this comment.
Perhaps a slight performance benefit. But that's not the goal of the change. It just avoids the potential for hitting a (very unlikely) race condition by just attempting the operation and catching if it fails instead of checking if the operation can be performed then doing the operation if it can be. Does that make sense?
This was the exact topic of discussion for fs.exists() within the Standard Library. See the note in https://jsr.io/@std/fs/doc/~/exists.
| async isDirectory(dir) { | ||
| return Object.keys(files).some((file) => file.startsWith(dir + "/")); | ||
| }, |
There was a problem hiding this comment.
Note: this mock is no longer exists, but that seems fine as tests pass.
Mrashes
left a comment
Mrashes
left a comment
There was a problem hiding this comment.
Overall change makes sense - as I'm not a contributor I don't feel confident in approving but good job on this!
| "foo.txt": "foo", | ||
| }); | ||
| const transformer = new FreshFileTransformer(); | ||
| using _denoReadFileStub = stubDenoReadFile("foo"); |
There was a problem hiding this comment.
first time seeing the using keyword, neat!
| try { | ||
| for await ( | ||
| const entry of walk(islandDir, { | ||
| includeDirs: false, | ||
| exts: ["tsx", "jsx", "ts", "js"], | ||
| skip, | ||
| }) | ||
| ) { | ||
| islandPaths.push(entry.path); | ||
| } | ||
| } catch (error) { | ||
| if (!(error instanceof Deno.errors.NotFound)) { | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| for await ( | ||
| const entry of internals.walk(routesDir, { | ||
| includeDirs: false, | ||
| exts: ["tsx", "jsx", "ts", "js"], | ||
| skip, | ||
| }) | ||
| ) { | ||
| const relative = path.relative(routesDir, entry.path); | ||
|
|
||
| // A `(_islands)` path segment is a local island folder. | ||
| // Any route path segment wrapped in `(_...)` is ignored | ||
| // during route collection. | ||
| const match = relative.match(GROUP_REG); | ||
| if (match && match[2][0] === "_") { | ||
| if (match[2] === "_islands") { | ||
| islandPaths.push(entry.path); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| const url = new URL(relative, "http://localhost/"); | ||
| relRoutePaths.push(url.pathname.slice(1)); | ||
| }, | ||
| ignore, | ||
| fs, | ||
| ), | ||
| ]); | ||
| const url = new URL(relative, "http://localhost/"); | ||
| relRoutePaths.push(url.pathname.slice(1)); | ||
| } | ||
| } catch (error) { | ||
| // `islandDir` or `routesDir` does not exist, so we can skip it | ||
| if (!(error instanceof Deno.errors.NotFound)) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there any performance gain from avoiding the isDirectory function?
|
@marvinhagemeister PTAL |
|
While this makes testing and implementation much simpler, closing as stale. |
2 participants
These changes simplify implementation logic and mock FS APIs in a more direct manner, making things easier to reason about.